Deeply normalize param env in compare_impl_item if using the next solver
#149345
Conversation
|
Some changes occurred to the core trait solver cc @rust-lang/initiative-trait-system-refactor |
rustbot
added
S-waiting-on-review
This comment has been minimized.
This comment has been minimized.
| // An unnormalized env is better than nothing. | ||
| debug!("normalize_param_env_or_error: errored resolving outlives predicates"); | ||
| return elaborated_env; | ||
| }; |
There was a problem hiding this comment.
don't need to split outlives vs non-outlives predicate norm in the new solver. This only matters for the old one
There was a problem hiding this comment.
I think that way you also don't need a separate do_deeply_normalize_predicates function as you only call it once
| drop(infcx.take_registered_region_obligations()); | ||
| drop(infcx.take_registered_region_assumptions()); | ||
| drop(infcx.take_and_reset_region_constraints()); |
There was a problem hiding this comment.
hmm, this feels... really bad xd
like, that's kind of the point of what we're doing here, but still
Does not dropping these things and instead not emitting a span_delayed_bug if there are errors also work? And also add an explicit comment referencing the FIXME from the function docs
|
|
||
| debug!("normalize_param_env_or_error: elaborated-predicates={:?}", predicates); | ||
|
|
||
| let elaborated_env = ty::ParamEnv::new(tcx.mk_clauses(&predicates)); |
There was a problem hiding this comment.
don't think we have to create a new param_env at all here 🤔 unsure why we do it in the old solver...
wanna update the new solver to just use the unnormalized_env instead?
| // FIXME(-Zhigher-ranked-assumptions): this is a hack to walk around the fact that we don't support | ||
| // placeholder assumptions right now. | ||
| // We should remove this once we have proper support for implied bounds on binders. | ||
| /// Deeply normalize the param env using the next solver. |
There was a problem hiding this comment.
| /// Deeply normalize the param env using the next solver. | |
| /// Deeply normalize the param env using the next solver ignoring | |
| /// region errors. | |
| /// | |
| /// FIXME(-Zhigher-ranked-assumptions): this is a hack to work around | |
| /// the fact that we don't support placeholder assumptions right now | |
| /// and is necessary for `compare_method_predicate_entailment`, see the | |
| /// use of this function for more info. We should remove this once we | |
| /// have proper support for implied bounds on binders. |
elaborated a bit and also want the FIXME to be part of the doc comment. The fact that this function is somewhat scuffed and shouldn't be used should be visible in the docs
| // FIXME: Temporary placeholders may get added to infcx's region constraints/obligations, | ||
| // which can cause problem for `resolve_regions`. |
There was a problem hiding this comment.
what do you mean here? I agree that we may get constraints involving placeholders from this, but that's fine™
We have the same behavior when relating binders and what not. The way we handle such placeholder constraints may change going forward and ideally we'd eagerly handle all of them when "exiting" the binder again, but for now "leaking" these constraints is normal
| // We should remove this once we have proper support for implied bounds on binders. | ||
| /// Deeply normalize the param env using the next solver. | ||
| #[instrument(level = "debug", skip(tcx))] | ||
| pub fn deeply_normalize_param_env_or_error<'tcx>( |
There was a problem hiding this comment.
| pub fn deeply_normalize_param_env_or_error<'tcx>( | |
| pub fn deeply_normalize_param_env_ignoring_regions<'tcx>( |
| let param_env = traits::normalize_param_env_or_error(tcx, param_env, normalize_cause); | ||
| // FIXME(-Zhigher-ranked-assumptions): lazy normalization may postpone region constraints to | ||
| // an infcx that checks regions. Deeply normalize the param env in the next solver as well. | ||
| // cc trait-system-refactor-initiative/issues/166. |
There was a problem hiding this comment.
| // cc trait-system-refactor-initiative/issues/166. | |
| // FIXME(-Zhigher-ranked-assumptions): The `hybrid_preds` | |
| // should be well-formed. However, using them may result in | |
| // region errors as we currently don't track placeholder | |
| // assumptions. | |
| // | |
| // To avoid being backwards incompatible with the old solver, | |
| // we also eagerly normalize the where-bounds in the new solver | |
| // here while ignoring region constraints. This means we can then | |
| // use where-bounds whose normalization results in placeholder | |
| // errors further down without getting any errors. | |
| // | |
| // It should be sound to do so as the only region errors here | |
| // should be due to missing implied bounds. | |
| // | |
| // cc trait-system-refactor-initiative/issues/166. |
elaborated a bit here
4 participants
Fixes rust-lang/trait-system-refactor-initiative#166.
Duplicated the
normalize_param_env_or_errorfunction to force deep normalization forcompare_impl_item.r? @lcnr